-
Notifications
You must be signed in to change notification settings - Fork 975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accounts Revamp Fixes: "Overall" Wallet Improvements #6736
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6736 +/- ##
==========================================
+ Coverage 60.07% 62.27% +2.19%
==========================================
Files 323 384 +61
Lines 27422 29823 +2401
==========================================
+ Hits 16473 18571 +2098
+ Misses 8733 8593 -140
- Partials 2216 2659 +443 |
…s/prysm into wallet-improvements-overall
walletDir string | ||
accountsPath string | ||
passwordsDir string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the difference between walletDir and accountsPath?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the wallet path is the path of the prysm-wallet-v2, and the accounts path is deeper within that directory
@@ -15,53 +15,55 @@ import ( | |||
// CreateWallet from user input with a desired keymanager. If a | |||
// wallet already exists in the path, it suggests the user alternatives | |||
// such as how to edit their existing wallet configuration. | |||
func CreateWallet(cliCtx *cli.Context) error { | |||
func CreateWallet(cliCtx *cli.Context) (*Wallet, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this returned wallet used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used as part of account create or import
Co-authored-by: Ivan Martinez <ivanthegreatdev@gmail.com>
…s/prysm into wallet-improvements-overall
wallet, err := OpenWallet(cliCtx) | ||
if err != nil { | ||
if errors.Is(err, ErrNoWalletFound) { | ||
return errors.Wrap(err, "no wallet found at path, create a new wallet with wallet-v2 create") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is the export code. Instead of directing the user to create a wallet, maybe we could just inform them there is nothing to export?
if err != nil { | ||
return errors.Wrap(err, "could not read keymanager kind for existing wallet") | ||
ctx := context.Background() | ||
wallet, err := createOrOpenWallet(cliCtx, func(cliCtx *cli.Context) (*Wallet, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you add a test case with where no wallet exists?
validator/accounts/v2/wallet_edit.go
Outdated
return errors.Wrap(err, "could not unmarshal config") | ||
} | ||
log.Infof("Current configuration") | ||
fmt.Printf("%s\n", cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm! Seems out of place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prints out the current configuration to stdout. Left a comment to clarify
What does this PR do? Why is it needed?
WalletPasswordsDirFlag
toedit-config
Which issues(s) does this PR fix?
Part of #6715
Other notes for review